Implement try_with_capacity and improve with_capacity - #574

I think the issue underlying the codebase is much bigger than "we had an unoptimized implementation"

if you think about it, why is that faster than what we had?? well, it directly gives information to the compiler of some invariants and changes code paths knowing that

before, with_capacity didn't know that the instance could never be spilled and it's length must equal zero because we just created it, so we had additional checks and stuff

I don't think this is an issue of "we didn't optimize this function", it is "we have a shitload of methods and implementations on a 2.5k LOC file and surprisingly many of them aren't even doing what they are supposed to do in terms of the abstraction"

this may be a bit extreme but I'm proposing a lib.rs rewrite from scratch. lib.rs is currently blocking:

• simplify the TaggedLen / set_len / capacity ... model #451: we can't change without breaking everything
• modularize associated iterators #479: public iterator API and internals are all mixed up in use
• change RawSmallVec heap to NonNull<[MaybeUninit<T>]> #441: outright impossible

and we can't even trust the current implementations in it to be neither optimal nor fully safe (remember that memory leak that appeared from nowhere in #450 or the stacked borrow violation in #449)

I haven't been opening PRs recently because I have like 5 branches discarded and 20 half-implementations that came across restructuring errors (e.g. manual iterator field magic, spec_traits on random places, etc). this has also happened to some of you

more than a 2k LOC replacement PR it would look more like: delete most lib.rs, comment everything that is giving errors elsewhere, and then reimplement everything one step at a time to bring it all online

if modularization has shown something it's that simple modules were easily extracted, core stuff was all intertwined

this optimization originated in #500 (comment)

we actually shouldn't be optimizing in an alpha version. we are still in alpha because we are missing features, https://github.com/servo/rust-smallvec/wiki/on-v2%E2%80%90alpha-performance-contributions

Request For Comments @bolshoytoster @TDecking @fereidani

I have a problem with not optimizing in alpha stage, I experienced that it is hard to optimize a project when it is nearly finished and already have made wrong choices (explained my opinion in https://fereidani.com/optimization-first-rule-dont-but-aim-for-it).
If we delay optimization, when we will have time to review these unsafe codes? during beta? what if optimization requires structural changes or other rewrites?
But I believe we need to have a standard about what optimization code should get merged, for example threshold of how many lines it adds and what affects it has on the binary size of the project(how many instructions add or removed).
Following standard library path is the best guideline at least for now.
I was considering adding unreachable_unchecked to these reserve functions for further improvements too.
They have massive gains by giving compiler hints about the success/need of reallocation.
These capacity related optimizations have huge impact on performance of the smallvec and we should consider implementing them, both hot and cold variants. we can refactor them to another file to keep them contained if you are worried about lines of code.
I generally believe any optimization that 1.reduces instruction counts and 2.improves performance must get merged if and only if proved completely sound.

I agree with @fereidani

that's the stance I'm also defending

though I'm not in favor of optimizing what we have. why doesn't reserve use unreachable_unchecked already for example?? why didn't the original implementation already come with it?? that should have been done

but instead of making things one I have the impression we are chasing our tail by repeatedly optimizing stuff instead of committing a really good implementation

why are we fixing technical debt that's deep into the repo??

honestly the "no performance contributions in alpha" was mostly an ad-hoc rule to focus on architecture first (length, modularization) and avoid trying to fixate ourselves on making small functions a bit faster. the endless structs and assert functions were also pretty annoying, not because they are bad by themselves, but because having them inline is noisy and a module would have been preferred instead, I am in favor of doing so

If we delay optimization, when we will have time to review these unsafe codes? during beta? what if optimization requires structural changes or other rewrites?

why not make it once and properly instead of having to review and refactor code that's four years old whilst having to remake core parts of the architecture??

why don't we center our architecture first around performance and not let anything in that's half-baked?? and we are suddenly realizing we are leaving massive gains everywhere??

what I'm most worried about is that when I have to implement something or review something, there's no "default obvious right way of doing so that is fast", but surprisingly 10 different ways with 4 different helper methods that all do pretty much the same except that three do not make assumptions that can be made and are slower

about the hot and cold variants, I've never gone deep into that stuff so it looks a bit weird for me, but I guess it makes sense. it's annoying to me but I can understand it, it will make sense once I get more used into that

every time you try to understand a new function, like retain_mut now, that implements really good optimizations, you need to discover on-the-fly what kinds of optimizations it uses and who knows whether other functions use the same ones. we have a ton of structs that do stuff on drop for optimizing code / better panic handling, why don't we have them somewhere that we can understand them and make sense of them as a whole

my suggestion is to make a rewrite with the optimizations (which often just means a good architecture and the micro-stuff) instead of having to audit the ~30 possible methods that are not optimized and that do right-but-slightly-unrelated stuff like the current implementation this PR is trying to change

by the way, that post is great. I haven't read it fully but it's really good

that's the stance I'm also defending

Great!

though I'm not in favor of optimizing what we have. why doesn't reserve use unreachable_unchecked already for example?? why didn't the original implementation already come with it?? that should have been done

Many of these optimizations are new even to the rust standard library and are results of many years of experience of rust community and developers, we can't judge them, they did the best they could in their time, we need to do our best now.

why are we fixing technical debt that's deep into the repo??

Repo itself is valuable in two sense, first it is heavily tested code with more than 1 billion downloads running currently on billions of devices, second we should respect its history and previous contributors, they put great amount of time and effort achieving what it does.
IMHO our task is to improve it and make it better, not rebuild it from scratch, retest it again and walk the same road again.
It's is much easier to transform a good code than rewrite it, this library code quality is good, its adaptation is the evidence for it. maybe needs some refactor, but code quality itself is good.

why don't we center our architecture first around performance and not let anything in that's half-baked?? and we are suddenly realizing we are leaving massive gains everywhere??

I think we are nearly finished with major perf changes that need restructuring. from now on most perf related PRs will not introduce more lines than what they remove.

what I'm most worried about is that when I have to implement something or review something, there's no "default obvious right way of doing so that is fast", but surprisingly 10 different ways with 4 different helper methods that all do pretty much the same except that three do not make assumptions that can be made and are slower

If we have same functions doing same thing, we must refactor, even at cost of slight performance(sub percent).
I always prefer maintainable code to extremely optimized code that is not maintainable.

Also these links can help you decide when these hard decisions are available(in case you haven't read them):
https://nnethercote.github.io/perf-book/
https://std-dev-guide.rust-lang.org/development/perf-benchmarking.html

As this library is generic mostly, you can't count assembly instructions directly(count test and bench binary instructions instead), there are some evidence that help you decide really fast if a PR worth merging(from top of my mind, things that I would consider).

If it improves performance
Good points:

1. Does it reduce lines of code?
2. Does it reduce asm instructions in inline functions?
3. Does it apply to general use case of this library?

Bad points:

1. Does it optimize to be successful only in a micro-benchmark by ignoring general use-case?
2. Does it sacrifice code clarity for small gain?
3. Does it make wrong assumption about the use case of users?

about the hot and cold variants, I've never gone deep into that stuff so it looks a bit weird for me, but I guess it makes sense. it's annoying to me but I can understand it, it will make sense once I get more used into that

It shows a good quality about you, when you are unsure, don't merge it. take your time, analyse it and return back to it.

every time you try to understand a new function, like retain_mut now, that implements really good optimizations, you need to discover on-the-fly what kinds of optimizations it uses and who knows whether other functions use the same ones. we have a ton of structs that do stuff on drop for optimizing code / better panic handling, why don't we have them somewhere that we can understand them and make sense of them as a whole

I'm not sure about this that this is going to improve anything. They are usually independent optimizations with different algorithms.

my suggestion is to make a rewrite with the optimizations (which often just means a good architecture and the micro-stuff) instead of having to audit the ~30 possible methods that are not optimized and that do right-but-slightly-unrelated stuff like the current implementation this PR is trying to change

I would not go that path, we will have to audit much harder, and we don't have luxury of this heavily tested code with that. I think we would have much harder time rewriting it from scratch.

by the way, that post is great. I haven't read it fully but it's really good

Thank you, Really glad that you liked it. I've updated it today once again, it was missing some cold and inlining guides, and had some minor issues.